-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't mark non-varied constant params #1148
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
7cb5d2b
to
d39b683
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1148 +/- ##
==========================================
+ Coverage 94.56% 94.58% +0.01%
==========================================
Files 51 51
Lines 8947 8972 +25
==========================================
+ Hits 8461 8486 +25
Misses 486 486
|
63d431f
to
3c1a59d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 19. Check the log or trigger a new build to see more.
bool HasAnalysisRun = false; | ||
} m_ActivityRunInfo; | ||
|
||
public: | ||
static std::set<const clang::VarDecl*> AllVariedDecls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'AllVariedDecls' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static std::set<const clang::VarDecl*> AllVariedDecls;
^
QualType innermost = VDTy; | ||
while (innermost->isPointerType()) | ||
innermost = innermost->getPointeeType(); | ||
if (VDTy->isPointerType() && !innermost.isConstQualified()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: repeated branch in conditional chain [bugprone-branch-clone]
if (VDTy->isPointerType() && !innermost.isConstQualified()) {
^
Additional context
lib/Differentiator/ActivityAnalyzer.cpp:165: end of the original
} else if (VDTy->isArrayType()) {
^
lib/Differentiator/ActivityAnalyzer.cpp:165: clone 1 starts here
} else if (VDTy->isArrayType()) {
^
lib/Differentiator/DiffPlanner.cpp
Outdated
} else if (isa<FunctionDecl>(DRE->getDecl())) | ||
m_FnDRE = DRE; | ||
return false; | ||
std::set<const clang::VarDecl*> DiffRequest::AllVariedDecls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'AllVariedDecls' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
std::set<const clang::VarDecl*> DiffRequest::AllVariedDecls;
^
lib/Differentiator/DiffPlanner.cpp
Outdated
return false; | ||
std::set<const clang::VarDecl*> DiffRequest::AllVariedDecls; | ||
|
||
static SourceLocation noLoc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'noLoc' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static SourceLocation noLoc;
^
/// \param SemaRef Reference to Sema | ||
DeclRefExpr* getArgFunction(CallExpr* call, Sema& SemaRef) { | ||
struct Finder : RecursiveASTVisitor<Finder> { | ||
Sema& m_SemaRef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member 'm_SemaRef' of type 'Sema &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
Sema& m_SemaRef;
^
: m_SemaRef(SemaRef), m_BeginLoc(beginLoc) {} | ||
|
||
// Required for visiting lambda declarations. | ||
bool shouldVisitImplicitCode() const { return true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'shouldVisitImplicitCode' should be marked [[nodiscard]] [modernize-use-nodiscard]
bool shouldVisitImplicitCode() const { return true; } | |
[[nodiscard]] bool shouldVisitImplicitCode() const { return true; } |
: m_SemaRef(SemaRef), m_BeginLoc(beginLoc) {} | ||
|
||
// Required for visiting lambda declarations. | ||
bool shouldVisitImplicitCode() const { return true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: method 'shouldVisitImplicitCode' can be made static [readability-convert-member-functions-to-static]
bool shouldVisitImplicitCode() const { return true; } | |
static bool shouldVisitImplicitCode() { return true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
|
||
// Emit error diagnostics | ||
if (R.empty()) { | ||
const char diagFmt[] = "'%0' has no defined operator()"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
const char diagFmt[] = "'%0' has no defined operator()";
^
m_SemaRef.Diag(m_BeginLoc, diagId) << RD->getName(); | ||
return false; | ||
} else if (!R.isSingleResult()) { | ||
const char diagFmt[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
const char diagFmt[] =
^
lib/Differentiator/DiffPlanner.cpp
Outdated
// Emit diagnostics for candidate functions | ||
for (auto oper = R.begin(), operEnd = R.end(); oper != operEnd; | ||
++oper) { | ||
auto candidateFn = cast<CXXMethodDecl>(oper.getDecl()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'auto candidateFn' can be declared as 'auto *candidateFn' [llvm-qualified-auto]
auto candidateFn = cast<CXXMethodDecl>(oper.getDecl());
^
this fix will not be applied because it overlaps with another fix
} else if (R.isSingleResult() == 1 && | ||
cast<CXXMethodDecl>(R.getFoundDecl())->getAccess() != | ||
AccessSpecifier::AS_public) { | ||
const char diagFmt[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
const char diagFmt[] =
^
3d2263b
to
d63cb79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 28. Check the log or trigger a new build to see more.
@@ -3,12 +3,12 @@ | |||
|
|||
#include "clang/AST/RecursiveASTVisitor.h" | |||
#include "llvm/ADT/SmallSet.h" | |||
#include <iterator> | |||
#include <set> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: included header iterator is not used directly [misc-include-cleaner]
#include <set> | |
#include <set> |
@@ -34,11 +35,13 @@ | |||
} m_TbrRunInfo; | |||
|
|||
mutable struct ActivityRunInfo { | |||
std::set<const clang::VarDecl*> ToBeRecorded; | |||
std::set<const clang::VarDecl*> VariedDecls; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::VarDecl" is directly included [misc-include-cleaner]
include/clad/Differentiator/DiffPlanner.h:5:
- #include <iterator>
+ #include <clang/AST/Decl.h>
+ #include <iterator>
bool HasAnalysisRun = false; | ||
} m_ActivityRunInfo; | ||
|
||
public: | ||
const DerivativeBuilder* Builder = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member variable 'Builder' has public visibility [cppcoreguidelines-non-private-member-variables-in-classes]
const DerivativeBuilder* Builder = nullptr;
^
@@ -144,6 +148,16 @@ | |||
|
|||
bool shouldBeRecorded(clang::Expr* E) const; | |||
bool shouldHaveAdjoint(const clang::VarDecl* VD) const; | |||
|
|||
void setVariedDecls(std::set<const clang::VarDecl*> init) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: the parameter 'init' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]
void setVariedDecls(std::set<const clang::VarDecl*> init) { | |
void setVariedDecls(const std::set<const clang::VarDecl*>& init) { |
@@ -144,6 +148,16 @@ | |||
|
|||
bool shouldBeRecorded(clang::Expr* E) const; | |||
bool shouldHaveAdjoint(const clang::VarDecl* VD) const; | |||
|
|||
void setVariedDecls(std::set<const clang::VarDecl*> init) { | |||
for (auto* vd : init) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'auto *vd' can be declared as 'const auto *vd' [readability-qualified-auto]
for (auto* vd : init) | |
for (const auto* vd : init) |
std::set<const clang::VarDecl*> getVariedDecls() const { | ||
return this->m_ActivityRunInfo.VariedDecls; | ||
} | ||
DiffRequest() : Builder(nullptr) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member initializer for 'Builder' is redundant [cppcoreguidelines-use-default-member-init]
DiffRequest() : Builder(nullptr) {} | |
DiffRequest() : {} |
MutableArrayRef<ParmVarDecl*> FDparam = FD->parameters(); | ||
for (std::size_t i = 0, e = CE->getNumArgs(); i != e; ++i) { | ||
clang::Expr* par = CE->getArg(i); | ||
|
||
QualType parType = FDparam[i]->getType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::QualType" is directly included [misc-include-cleaner]
lib/Differentiator/ActivityAnalyzer.cpp:1:
+ #include <clang/AST/Type.h>
if (m_Varied) | ||
m_VariedDecls.insert(FDparam[i]); | ||
else if ((parType->isReferenceType() || | ||
(utils::isArrayOrPointerType(parType) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clad::utils::isArrayOrPointerType" is directly included [misc-include-cleaner]
lib/Differentiator/ActivityAnalyzer.cpp:1:
+ #include <clad/Differentiator/CladUtils.h>
return true; | ||
} | ||
|
||
bool VariedAnalyzer::VisitDeclStmt(DeclStmt* DS) { | ||
for (Decl* D : DS->decls()) { | ||
|
||
QualType VDTy = cast<VarDecl>(D)->getType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::cast" is directly included [misc-include-cleaner]
QualType VDTy = cast<VarDecl>(D)->getType();
^
m_FnDRE = DRE; | ||
return false; | ||
// std::set<const clang::VarDecl*> DiffRequest::AllVariedDecls; | ||
static SourceLocation noloc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::SourceLocation" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <clang/Basic/SourceLocation.h>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 20. Check the log or trigger a new build to see more.
m_FnDRE = DRE; | ||
return false; | ||
// std::set<const clang::VarDecl*> DiffRequest::AllVariedDecls; | ||
static SourceLocation noloc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'noloc' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
static SourceLocation noloc;
^
/// \param SemaRef Reference to Sema | ||
DeclRefExpr* getArgFunction(CallExpr* call, Sema& SemaRef) { | ||
struct Finder : RecursiveASTVisitor<Finder> { | ||
Sema& m_SemaRef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: member 'm_SemaRef' of type 'Sema &' is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]
Sema& m_SemaRef;
^
: m_SemaRef(SemaRef), m_BeginLoc(beginLoc) {} | ||
|
||
// Required for visiting lambda declarations. | ||
bool shouldVisitImplicitCode() const { return true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'shouldVisitImplicitCode' should be marked [[nodiscard]] [modernize-use-nodiscard]
bool shouldVisitImplicitCode() const { return true; } | |
[[nodiscard]] bool shouldVisitImplicitCode() const { return true; } |
: m_SemaRef(SemaRef), m_BeginLoc(beginLoc) {} | ||
|
||
// Required for visiting lambda declarations. | ||
bool shouldVisitImplicitCode() const { return true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: method 'shouldVisitImplicitCode' can be made static [readability-convert-member-functions-to-static]
bool shouldVisitImplicitCode() const { return true; } | |
static bool shouldVisitImplicitCode() { return true; } |
bool shouldVisitImplicitCode() const { return true; } | ||
|
||
bool VisitDeclRefExpr(DeclRefExpr* DRE) { | ||
if (auto* VD = dyn_cast<VarDecl>(DRE->getDecl())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::dyn_cast" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <clang/Basic/LLVM.h>
} | ||
} else if (isa<FunctionDecl>(DRE->getDecl())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::isa" is directly included [misc-include-cleaner]
} else if (isa<FunctionDecl>(DRE->getDecl()))
^
bool VisitCXXRecordDecl(CXXRecordDecl* RD) { | ||
auto callOperatorDeclName = | ||
m_SemaRef.getASTContext().DeclarationNames.getCXXOperatorName( | ||
OverloadedOperatorKind::OO_Call); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::OverloadedOperatorKind" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <clang/Basic/OperatorKinds.h>
|
||
// Emit error diagnostics | ||
if (R.empty()) { | ||
const char diagFmt[] = "'%0' has no defined operator()"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
const char diagFmt[] = "'%0' has no defined operator()";
^
DiagnosticsEngine::Level::Error, diagFmt); | ||
m_SemaRef.Diag(m_BeginLoc, diagId) << RD->getName(); | ||
} else if (!R.isSingleResult()) { | ||
const char diagFmt[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
const char diagFmt[] =
^
// Emit diagnostics for candidate functions | ||
for (auto oper = R.begin(), operEnd = R.end(); oper != operEnd; | ||
++oper) { | ||
auto* candidateFn = cast<CXXMethodDecl>(oper.getDecl()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::cast" is directly included [misc-include-cleaner]
auto* candidateFn = cast<CXXMethodDecl>(oper.getDecl());
^
d349c5a
to
d255a1f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
} | ||
} else if (R.isSingleResult() == 1 && | ||
cast<CXXMethodDecl>(R.getFoundDecl())->getAccess() != | ||
AccessSpecifier::AS_public) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::AccessSpecifier" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <clang/Basic/Specifiers.h>
} else if (R.isSingleResult() == 1 && | ||
cast<CXXMethodDecl>(R.getFoundDecl())->getAccess() != | ||
AccessSpecifier::AS_public) { | ||
const char diagFmt[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
const char diagFmt[] =
^
for (auto* decl : RD->decls()) { | ||
if (decl == callOperator) | ||
break; | ||
if (isa<AccessSpecDecl>(decl)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::AccessSpecDecl" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <clang/AST/DeclCXX.h>
return finder.m_FnDRE; | ||
} | ||
// Emit diagnostics for the found call operator | ||
m_SemaRef.Diag(callOperator->getBeginLoc(), diag::note_access_natural) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::diag::note_access_natural" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <clang/Basic/DiagnosticSema.h>
<< isImplicit; | ||
|
||
} else { | ||
assert(R.isSingleResult() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "assert" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <cassert>
@@ -237,7 +225,7 @@ | |||
// Add the "&" operator | |||
auto* newUnOp = | |||
SemaRef | |||
.BuildUnaryOp(nullptr, noLoc, UnaryOperatorKind::UO_AddrOf, DRE) | |||
.BuildUnaryOp(nullptr, noloc, UnaryOperatorKind::UO_AddrOf, DRE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "clang::UnaryOperatorKind" is directly included [misc-include-cleaner]
lib/Differentiator/DiffPlanner.cpp:20:
+ #include <clang/AST/OperationKinds.h>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
std::set<const clang::VarDecl*> getVariedDecls() const { | ||
return this->m_ActivityRunInfo.VariedDecls; | ||
} | ||
DiffRequest() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]
DiffRequest() {} | |
DiffRequest() = default; |
m_ActivityRunInfo.ToBeRecorded.end())); | ||
|
||
if (Builder) | ||
for (auto diffreq : this->Builder->m_DiffRequestGraph.getNodes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
for (auto diffreq : this->Builder->m_DiffRequestGraph.getNodes()) | |
for (const auto& diffreq : this->Builder->m_DiffRequestGraph.getNodes()) |
|
||
if (Builder) | ||
for (auto diffreq : this->Builder->m_DiffRequestGraph.getNodes()) | ||
for (auto vd : diffreq.getVariedDecls()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'auto vd' can be declared as 'const auto *vd' [llvm-qualified-auto]
for (auto vd : diffreq.getVariedDecls()) | |
for (const auto *vd : diffreq.getVariedDecls()) |
No description provided.